Skip to content
This repository was archived by the owner on Sep 5, 2025. It is now read-only.

fix: add global Shutdown function; allow tests to pass on Windows regardless of temp folder state after tests#98

Merged
matthewmcneely merged 14 commits intomainfrom
matthewmcneely/add-shutdown-func
May 28, 2025
Merged

fix: add global Shutdown function; allow tests to pass on Windows regardless of temp folder state after tests#98
matthewmcneely merged 14 commits intomainfrom
matthewmcneely/add-shutdown-func

Conversation

@matthewmcneely
Copy link
Contributor

Description

This tortured PR allows tests to complete on Windows.

Background:

  • Dgraph's pstore/walstore."Close" methods do not actually close the write ahead logs
  • Go tooling's TempDir() function (which deletes the contents of the allocated folder) fails in Windows because the file handles are still active

This PR adds a Windows-specific TempDir function which tries to delete the folder, but will ultimately fail. But the tests pass.

Note: Windows users will need to manually delete folders created during testing. The folders take the form of C:\Users\<USER_NAME>\AppData\Local\Temp\modusgraph_test_*

Checklist

  • Code compiles correctly and linting passes locally

@matthewmcneely matthewmcneely requested review from a team and Copilot May 27, 2025 22:06
@matthewmcneely matthewmcneely enabled auto-merge (squash) May 27, 2025 22:06
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes issues with tests failing on Windows by introducing a new global Shutdown function and a Windows‑specific temporary directory helper.

  • Introduces a GetTempDir function that applies extra cleanup steps on Windows.
  • Replaces mg.ResetSingleton() calls with mg.Shutdown() for proper engine cleanup.
  • Updates tests across multiple files to use GetTempDir for file-based URIs.

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
util_test.go Added GetTempDir and updated engine cleanup to use Shutdown.
update_test.go, query_test.go, insert_test.go, delete_test.go, client_test.go, admin_test.go Replaced t.TempDir() with GetTempDir(t) for better Windows support.
engine.go Introduced activeEngine tracking and Shutdown functionality; updated error messages.
unit_test/api_test.go Removed TestUserStore, potentially impacting test coverage.
CHANGELOG.md Updated changelog to document the new Shutdown function.
Comments suppressed due to low confidence (1)

unit_test/api_test.go:1102

  • The removal of TestUserStore might reduce coverage of user store functionality. Please verify that this functionality is adequately tested elsewhere or that its removal is intentional.
func TestUserStore(t *testing.T) {

@trunk-io
Copy link

trunk-io bot commented May 27, 2025

Running Code Quality on PRs by uploading data to Trunk will soon be removed. You can still run checks on your PRs using trunk-action - see the migration guide for more information.

@ryanfoxtyler
Copy link
Member

@matthewmcneely am I understanding correctly that this avoids the test failure, but running ModusGraph on Windows could still produce unexpected results?

@matthewmcneely
Copy link
Contributor Author

@matthewmcneely am I understanding correctly that this avoids the test failure, but running ModusGraph on Windows could still produce unexpected results?

@ryanfoxtyler
The top-level tests pass on Parallels/Windows 11 which proves at least that much is working. I also ran the example apps on Parallels without error. It's just that I have NOT put in the hundreds of "reps" on Windows as I have on OSX. Given that, and the historical problems of Badger on Windows, I suggest we put forth that it's fine for local development, but for production we'd recommend Linux.

@matthewmcneely matthewmcneely merged commit e0f0f3a into main May 28, 2025
7 checks passed
@matthewmcneely matthewmcneely deleted the matthewmcneely/add-shutdown-func branch May 28, 2025 22:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants